Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR discourse#36082

Type: Corrupted (contains bugs)

Original PR Title: UX: One step wizard
Original PR Description: Consolidates all three wizard screens into one. Cleans up some leftover, unused wizard elements (checkbox, image, canvas inputs).

Internal ticket: t/164852

CleanShot 2025-11-25 at 10 35 43@2x

Original PR URL: discourse#36082


PR Type

Enhancement


Description

  • Consolidates three-step wizard into single unified setup step

  • Removes unused wizard elements (canvas, sidebar, step counter, navigation buttons)

  • Simplifies frontend component logic and styling

  • Removes refresh_required mechanism and multi-step navigation

  • Cleans up serializers by removing unused attributes


Diagram Walkthrough

flowchart LR
  A["Three-Step Wizard<br/>introduction → privacy → ready"] -->|Consolidate| B["Single Setup Step<br/>All fields in one step"]
  B -->|Remove| C["Navigation Elements<br/>next/previous buttons"]
  B -->|Remove| D["UI Elements<br/>canvas, sidebar, counter"]
  B -->|Simplify| E["Direct Completion<br/>Jump In → Home"]
Loading

File Walkthrough

Relevant files
Enhancement
12 files
builder.rb
Consolidate three wizard steps into one                                   
+17/-63 
wizard_step_serializer.rb
Remove next/previous navigation attributes                             
+2/-27   
field.rb
Remove icon attribute from field classes                                 
+2/-6     
wizard_field_serializer.rb
Remove unused field serializer attributes                               
+1/-35   
step_updater.rb
Remove refresh_required mechanism                                               
+2/-7     
wizard_field_choice_serializer.rb
Remove icon from choice serializer                                             
+1/-9     
steps_controller.rb
Simplify response handling and status codes                           
+1/-2     
step.rb
Remove description_vars from step attributes                         
+1/-1     
wizard.js
Update wizard model for single setup step                               
+6/-10   
wizard.scss
Remove multi-step styling and simplify layout                       
+6/-227 
wizard-step.gjs
Simplify step component to single-step flow                           
+28/-243
step.gjs
Simplify step template wrapper component                                 
+4/-57   
Tests
6 files
wizard_spec.rb
Update wizard tests for single-step flow                                 
+32/-25 
step_updater_spec.rb
Refactor updater specs for unified setup step                       
+19/-39 
wizard_builder_spec.rb
Update builder specs for single setup step                             
+11/-29 
wizard_serializer_spec.rb
Simplify serializer tests for one step                                     
+7/-7     
steps_controller_spec.rb
Update controller tests for setup endpoint                             
+3/-8     
wizard-test.js
Remove unused test helper imports                                               
+1/-121 
Documentation
2 files
server.en.yml
Update wizard step localization strings                                   
+4/-95   
client.en.yml
Remove unused wizard UI localization strings                         
+1/-80   
Additional files
13 files
checkbox.gjs +0/-37   
checkboxes.gjs +0/-55   
generic.gjs +0/-10   
index.js +0/-9     
logo-small.js +0/-94   
logo.js +0/-50   
image.gjs +0/-128 
index.js +0/-8     
-homepage-preview.js +0/-540 
-preview-base.gjs +0/-472 
index.gjs +0/-284 
wizard-canvas.gjs +0/-188 
wizard_step_spec.rb +0/-4     

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Audit Logs: The controller updates setup and privacy-related site settings but the new code adds no
logging of these critical configuration changes or who performed them.

Referred Code
def update
  wizard = Wizard::Builder.new(current_user).build
  updater = wizard.create_updater(params[:id], params[:fields])
  updater.update

  if updater.success?
    result = { success: "OK" }
    render json: result, status: :no_content
  else

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Weak Error Context: The new jumpIn flow sets a local error flag on validation failure without surfacing
actionable error details or recovery guidance, potentially reducing user feedback clarity.

Referred Code
@action
async jumpIn() {
  const valid = await this.step.validate();
  if (!valid) {
    this.hasError = true;
    return;
  }

  const response = await this.step.save();
  if (response && response.success) {
    // We are not using Ember routing here because we always want to reload the app
    // ensure language, site title are properly set.
    document.location = getUrl("/");
  }

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Minimal Validation: The new ensure_changed check and settings updates rely on provided params without visible
sanitization, which may be validated elsewhere but is not evident in the added code.

Referred Code
def ensure_changed(id)
  errors.add(id, "") if @fields[id].to_s == SiteSetting.defaults[id].to_s
end

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
The server response on successful update is incorrect

The update action in StepsController incorrectly returns a JSON body with a 204
No Content status. This violates HTTP standards and causes the frontend logic,
which expects a JSON response to confirm success, to fail.

Examples:

app/controllers/steps_controller.rb [16]
      render json: result, status: :no_content
frontend/discourse/app/static/wizard/components/wizard-step.gjs [48-53]
    const response = await this.step.save();
    if (response && response.success) {
      // We are not using Ember routing here because we always want to reload the app
      // ensure language, site title are properly set.
      document.location = getUrl("/");
    }

Solution Walkthrough:

Before:

# app/controllers/steps_controller.rb
class StepsController < ApplicationController
  def update
    # ...
    if updater.success?
      result = { success: "OK" }
      render json: result, status: :no_content # 204 No Content
    else
      # ...
    end
  end
end

// frontend/discourse/app/static/wizard/components/wizard-step.gjs
async jumpIn() {
  // ...
  const response = await this.step.save();
  if (response && response.success) { // This check fails
    document.location = getUrl("/"); // This redirect is never reached
  }
}

After:

# app/controllers/steps_controller.rb
class StepsController < ApplicationController
  def update
    # ...
    if updater.success?
      result = { success: "OK" }
      render json: result, status: :ok # 200 OK
    else
      # ...
    end
  end
end

// frontend/discourse/app/static/wizard/components/wizard-step.gjs
async jumpIn() {
  // ...
  const response = await this.step.save();
  if (response && response.success) { // This check now passes
    document.location = getUrl("/"); // Redirect occurs as expected
  }
}
Suggestion importance[1-10]: 9

__

Why: This is a critical bug that breaks the wizard's primary action, as the frontend redirect will fail due to an incorrect server response that violates HTTP specifications.

High
Possible issue
Fix broken client redirect logic

Change the success response status from 204 No Content to the default 200 OK to
ensure the JSON body is sent, which is required by the client for redirection.

app/controllers/steps_controller.rb [14-17]

 if updater.success?
   result = { success: "OK" }
-  render json: result, status: :no_content
+  render json: result
 else
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a functional bug introduced in the PR where the client-side redirect will fail because the server sends a 204 No Content response, while the client expects a JSON body.

High
Provide user feedback on save failure

In the jumpIn action, add an else block to handle failed save operations by
setting this.hasError = true, providing visual feedback to the user on
server-side validation failures.

frontend/discourse/app/static/wizard/components/wizard-step.gjs [41-54]

 async jumpIn() {
   const valid = await this.step.validate();
   if (!valid) {
     this.hasError = true;
     return;
   }
 
   const response = await this.step.save();
-  if (response && response.success) {
+  if (response?.success) {
     // We are not using Ember routing here because we always want to reload the app
     // ensure language, site title are properly set.
     document.location = getUrl("/");
+  } else {
+    this.hasError = true;
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where server-side validation errors are not handled, leaving the user without feedback. Adding an else block to set an error state is a critical improvement for user experience.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants